Skip to content

[EDITOR-512] Fix concurrent map read and map write #585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 29, 2021
Merged

[EDITOR-512] Fix concurrent map read and map write #585

merged 2 commits into from
Jan 29, 2021

Conversation

umbynos
Copy link
Contributor

@umbynos umbynos commented Jan 15, 2021

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • What kind of change does this PR introduce?

Bug fix

  • What is the current behavior?

Sometimes the agent randomly crashes with fatal error: concurrent map read and map write
The crashreport contains something like:

fatal error: concurrent map read and map write

goroutine 2178 [running]:
runtime.throw(0xd4c202, 0x21)
	/opt/hostedtoolcache/go/1.14.12/x64/src/runtime/panic.go:1116 +0x72 fp=0xc0007e7680 sp=0xc0007e7650 pc=0x4372f2
runtime.mapaccess2(0xc3f740, 0xc0001106c0, 0xc0004f40a0, 0xc0004f40a0, 0xc000630902)
	/opt/hostedtoolcache/go/1.14.12/x64/src/runtime/map.go:469 +0x258 fp=0xc0007e76c0 sp=0xc0007e7680 pc=0x411908
reflect.mapaccess(0xc3f740, 0xc0001106c0, 0xc0004f40a0, 0xd42538)
	/opt/hostedtoolcache/go/1.14.12/x64/src/runtime/map.go:1309 +0x3f fp=0xc0007e76f8 sp=0xc0007e76c0 pc=0x413b1f
reflect.Value.MapIndex(0xc3f740, 0xc0001106c0, 0x15, 0xc02c20, 0xc0004f40a0, 0x98, 0xd204e0, 0xc3f740, 0xc3f740)
	/opt/hostedtoolcache/go/1.14.12/x64/src/reflect/value.go:1173 +0x16d fp=0xc0007e7770 sp=0xc0007e76f8 pc=0x4986fd
encoding/json.mapEncoder.encode(0xd720b0, 0xc00049e400, 0xc3f740, 0xc0001106c0, 0x15, 0xc30100)
	/opt/hostedtoolcache/go/1.14.12/x64/src/encoding/json/encode.go:801 +0x305 fp=0xc0007e78e8 sp=0xc0007e7770 pc=0x517715
encoding/json.mapEncoder.encode-fm(0xc00049e400, 0xc3f740, 0xc0001106c0, 0x15, 0x1610100)
	/opt/hostedtoolcache/go/1.14.12/x64/src/encoding/json/encode.go:777 +0x64 fp=0xc0007e7928 sp=0xc0007e78e8 pc=0x5235e4
encoding/json.(*encodeState).reflectValue(0xc00049e400, 0xc3f740, 0xc0001106c0, 0x15, 0xc0007e0100)
	/opt/hostedtoolcache/go/1.14.12/x64/src/encoding/json/encode.go:358 +0x82 fp=0xc0007e7960 sp=0xc0007e7928 pc=0x514892
encoding/json.(*encodeState).marshal(0xc00049e400, 0xc3f740, 0xc0001106c0, 0x510100, 0x0, 0x0)
	/opt/hostedtoolcache/go/1.14.12/x64/src/encoding/json/encode.go:330 +0xf0 fp=0xc0007e79c0 sp=0xc0007e7960 pc=0x5143b0
encoding/json.Marshal(0xc3f740, 0xc0001106c0, 0xc0007e7ab0, 0xb8d4a1, 0xc000380a20, 0xc0007e7a98, 0xc0001bb400)
	/opt/hostedtoolcache/go/1.14.12/x64/src/encoding/json/encode.go:161 +0x52 fp=0xc0007e7a38 sp=0xc0007e79c0 pc=0x513732
github.com/arduino/arduino-create-agent/tools.(*Tools).writeMap(0x1616ae0, 0x29, 0xc00043cbc6)
	/home/runner/work/arduino-create-agent/arduino-create-agent/tools/tools.go:90 +0x42 fp=0xc0007e7ac0 sp=0xc0007e7a38 pc=0xb1f8e2
github.com/arduino/arduino-create-agent/tools.(*Tools).Download(0x1616ae0, 0xc0002eca6a, 0x7, 0xc0002eca5d, 0x6, 0xc0002eca64, 0x5, 0xc0002eca72, 0x4, 0x0, ...)
	/home/runner/work/arduino-create-agent/arduino-create-agent/tools/download.go:201 +0x13a3 fp=0xc0007e7ef8 sp=0xc0007e7ac0 pc=0xb1c123
main.checkCmd.func2(0xc0002eca50, 0x26)
	/home/runner/work/arduino-create-agent/arduino-create-agent/hub.go:194 +0x11c fp=0xc0007e7fd0 sp=0xc0007e7ef8 pc=0xb8ccac
runtime.goexit()
	/opt/hostedtoolcache/go/1.14.12/x64/src/runtime/asm_amd64.s:1373 +0x1 fp=0xc0007e7fd8 sp=0xc0007e7fd0 pc=0x466f41
created by main.checkCmd
	/home/runner/work/arduino-create-agent/arduino-create-agent/hub.go:165 +0x623
  • What is the new behavior?

This behavior should be fixed

  • Does this PR introduce a breaking change?

no

  • Other information:

In the process of fixing the bug I also updated the tools documentation

@umbynos umbynos self-assigned this Jan 15, 2021
@umbynos umbynos linked an issue Jan 15, 2021 that may be closed by this pull request
[BUG] Concurrent map write causes agent to crash on MacOs #494
Closed
@umbynos umbynos changed the title Fix concurrent map read and map write [EDITOR-512] Fix concurrent map read and map write Jan 15, 2021
umbynos
umbynos commented Jan 15, 2021
View reviewed changes
tools/tools.go
@@ -35,21 +36,30 @@ type Tools struct {
LastRefresh time.Time
Logger func(msg string)
installed map[string]string
mutex sync.RWMutex
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used RWMutex to handle the read/write access to the map.
I think this was a better approach than using the cleaner sync Map because it does not require changes in the code logic (using functions to interact with the new Map type)

@umbynos umbynos requested a review from zmoog January 15, 2021 09:52
@umbynos umbynos force-pushed the umbynos/fix_map_concurrency branch 3 times, most recently from 777c75f to 4025321 Compare January 20, 2021 10:05
zmoog
zmoog approved these changes Jan 26, 2021
View reviewed changes
Copy link
Contributor

@zmoog zmoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and does not require an update to internal APIs.

@umbynos umbynos force-pushed the umbynos/fix_map_concurrency branch from 4025321 to 0cb27d8 Compare January 29, 2021 15:57
@umbynos umbynos merged commit d09803d into devel Jan 29, 2021
@umbynos umbynos deleted the umbynos/fix_map_concurrency branch January 29, 2021 15:58
umbynos added a commit that referenced this pull request Jan 29, 2021
@umbynos
[EDITOR-512] Fix concurrent map read and map write (#585)
88e424a 
* update doc

* add read write mutex synchronization to installed map
@umbynos umbynos mentioned this pull request Apr 27, 2021
Merged
2 tasks
@rsora rsora added the type: imperfection Perceived defect in any part of project label Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmoog zmoog zmoog approved these changes

Assignees

@umbynos umbynos

Labels
type: imperfection Perceived defect in any part of project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Concurrent map write causes agent to crash on MacOs
3 participants
@umbynos @zmoog @rsora